feat(notice/toast): update styles for SHINE #2109
Conversation
🦋 Changeset detectedLatest commit: 0bc9779 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for stacks-svelte ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for stacks ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| // TODO: decouple .s-notice--btn from .s-btn | ||
| button&--dismiss { |
There was a problem hiding this comment.
Introduced s-notice--dismiss class here to make the dismiss button more explicit and style it accordingly.
| /** | ||
| * Whether to include a dismiss button on the notice or not | ||
| */ | ||
| dismissable: boolean; | ||
| /** | ||
| * Optional dismiss event handler | ||
| */ | ||
| onDismiss?: MouseEventHandler<HTMLButtonElement>; | ||
| /** | ||
| * Dismiss button label override | ||
| */ | ||
| i18nDismissButtonLabel?: string; | ||
There was a problem hiding this comment.
I made the dismiss button a boolean flag here with additional props instead of a special NoticeAction case like it was before. I think this make more sense and improves usability. Hopefully other folks agree 🙏🏾
| align-items: center; | ||
| gap: var(--su4); | ||
|
|
||
| &--icon { |
There was a problem hiding this comment.
Introduced the s-notice--icon class for the icons
mukunku
requested review from
CGuindon,
dancormier,
giamir and
ttaylor-stack
| <button type="button" class="s-link s-link__underlined">Undo</button> | ||
| <button type="button" class="s-link s-notice--dismiss js-toast-close" aria-label="Dismiss" data-action="s-toast#hide">{% icon "ClearSm" %}</button> |
There was a problem hiding this comment.
I opted to style the buttons with s-link but not sure if I should've used s-btn s-btn__link instead like we do in the Svelte component for Notices.
CGuindon
left a comment
CGuindon
left a comment
There was a problem hiding this comment.
Looking great, just noticing some weirdness when I resize the screen smaller:
- the whole browser starts to shrink instead of resize when I go below 600px width — feels like a docs issue but not sure if the toast is affecting this somehow
- Notices can wrap to multiple lines when needed:
- Padding right should maintain a minimum of 12px (even when there's no X or under button, the description text shouldn't touch the right edge of the notice)
- Links on the end shouldn't wrap within itself (keep as single line unless the whole description is wrapping)
- Code snipets shouldn't wrap within itself either
Added an extra example in the variations section to make that a bit clearer
For the toast not banner (possibly for the other ticket):
- I noticed when I add the
s-notice__importantto the toast example, the undo link and close icon stay black instead of turning white as well.
|
I also had a note to myself to write a warning note to add to the docs right below the Toast description text. Can you use a warning in-page notice there that says:
|
mukunku
changed the title
|
Thanks for the review @CGuindon ! I believe I addressed all your findings around responsiveness and multi-line content. Please take a look and let me know how it looks now. I also decided to do Toasts as part of this PR because the changes left were so small. I additionally added the warning you mentioned: |
CGuindon
left a comment
CGuindon
left a comment
There was a problem hiding this comment.
OOOooooooooh even some fancy stacking and animations (not that we see this often) — looking good!
FWIW That was already there; I didn't add that feature 😅 |
dancormier
left a comment
dancormier
left a comment
There was a problem hiding this comment.
Looking sharp 😎 I'd be happy to see this merged.
The only issue worth mentioning that I can see is with the margin-left on the button. In the Svelte Toast example for "With actions", I noticed the actions butting up against one another.
This is the sort of thing I'd expect to be resolved with a gap applied. The issue with this is it will add another xpx to the 22px margin-left on the notice button. I'd make a suggestion but I didn't have time to dig into it and I don't think this needs to hold up merging this PR.
Thanks for the nice work here @mukunku
Summary
This PR updates the Notice and Toast components according to the latest SHINE designs.
How to Test
To test the Notice component:
To test the Toast component: